Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add requires_file attr for py_wheel #644

Closed

Conversation

weixiao-huang
Copy link

@weixiao-huang weixiao-huang commented Mar 8, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla
Copy link

google-cla bot commented Mar 8, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@weixiao-huang weixiao-huang force-pushed the feat/add-requires-file branch 3 times, most recently from 0f95f75 to 28e48e7 Compare March 9, 2022 02:59
@pstradomski
Copy link
Collaborator

Any background for why this is needed? Maybe the documentation should be updated to reflect that and what's the interaction with "requires" attribute?

@weixiao-huang
Copy link
Author

Because sometimes we may write codes like this in setup.py

#!/usr/bin/env python

from setuptools import setup, find_packages

with open("requirements.txt") as fp:
    install_requires = fp.read()


setup(
    # get requires from file
    install_requires=install_requires,
)

However, in py_wheel, it only provides requires from bazel string. It seems we cannot get requires from files. So I add requires_file in py_wheel.

If requires specified, requires_file will not work

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Sep 9, 2022
@weixiao-huang
Copy link
Author

Is there any update of this PR?

@github-actions github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Sep 10, 2022
@UebelAndre
Copy link
Contributor

@weixiao-huang would you be able to rebase this and address conflicts?

@weixiao-huang weixiao-huang force-pushed the feat/add-requires-file branch 2 times, most recently from 35abc61 to feba774 Compare October 11, 2022 09:27
Copy link
Contributor

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no maintainer but did have some requests 😅

python/packaging.bzl Outdated Show resolved Hide resolved
tools/wheelmaker.py Outdated Show resolved Hide resolved
tools/wheelmaker.py Show resolved Hide resolved
if arguments.requires_file:
with open(arguments.requires_file) as fp:
additive_requires_list = ["Requires-Dist: {}".format(line) for line in fp.read().strip().split("\n")]
metadata += "\n" + "\n".join(additive_requires_list)
Copy link
Collaborator

@pstradomski pstradomski Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't that leave you with two \n in a row, so everything below will be interpreted as a description?

Can we have a test?

@@ -398,6 +403,10 @@ def main() -> None:
encoding="utf-8") as metadata_file:
metadata = metadata_file.read()

if arguments.requires_file:
with open(arguments.requires_file) as fp:
additive_requires_list = ["Requires-Dist: {}".format(line) for line in fp.read().strip().split("\n")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why read().strip().split() and not readlines()?

@github-actions
Copy link

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Apr 11, 2023
@chrislovecnm
Copy link
Collaborator

@weixiao-huang knudge

@github-actions github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Apr 24, 2023
@weixiao-huang
Copy link
Author

Sorry, I'll check it recently

@github-actions
Copy link

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Oct 22, 2023
Copy link

This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? Will close in 30 days if there is no new activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants